-
-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement listener subscription as async iterator protocol #20
Conversation
Just "xo": {
"parser": "babel-eslint"
}
Try adding the following below "presets": [
"@ava/stage-4"
] |
Great ! thanks. Still unable to run with a
//in async function context
const iterator = emitter.on('foo');
const {value,done} = await iterator.next(); // This works with transpiled and not transpiled code
//this does not work with transpiled code but works on supporting platforms
for await (let result of emitter.on('foo')){
//
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lorenzofox3 very neat! I wonder though whether this should be landed before there's a Node.js version that supports asynchronous iterators. Can the iteration protocol be implemented without using async function *
?
index.js
Outdated
if (queue.length > 0) { | ||
yield queue.shift(); | ||
} else { | ||
yield await emitter.once(eventName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't yield, queue
will contain the data of the next event after await
returns.
Alternatively you could await a new promise, and expose its resolve()
method to the listener created above. That way you don't need to add an additional listener.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right the yield is superfluous.
However I don't think exposing the resolve of the new promise on each iteration is a good idea: As the listener above is created once I would need to create a binding in its closure and reassign this binding to the resolve of the new promise on each iteration. That seems a bit awkward and unsafe, or I am missing something.
index.js
Outdated
this._getListeners(eventName).add(listener); | ||
return this.off.bind(this, eventName, listener); | ||
|
||
if (typeof listener === 'function') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this check the number of arguments passed, rather than the listener
type? It seems a little too easy to accidentally create an asynchronous iterator otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you wish. Although it is a public method with a specific contract so I think this alternative is better. Otherwise it means I'll need to check for "arguments" which is not correct in strict mode; Or use the spread operator on the signature which makes the contract less obvious in my opinion
You can use node 9 with For transpiler, I think TS has better async-iterator support than babel; it had the last time I checked. |
Until better support for async-iterator, it would be better to set it up as a helper in an other module, e.g. |
I have updated to pull request so the async iterator syntax behaviour has its own file.
|
@lorenzofox3 I've been playing with this a bit. I hope you don't mind if I push my commits onto your branch. Async iterators can be implemented without
I think we still need to resolve:
|
Don't worry it is all good !
Agree but the whole point if I understood correctly was to get the syntactic sugar of the
It is more my inability to run AvA with different configurations, I wanted to:
Regarding your implementation:
Again that is a matter of preference I guess.
Right I had not thought of an undefined variable. Then, I think another method like
probably.
yes (if it follows the same semantic than a the for of loop). It will call the the |
index.js
Outdated
}, | ||
return() { | ||
off(); | ||
queue = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be async too, take a value
argument and return {done: true, value}
.
https://tc39.github.io/proposal-async-iteration/#sec-asynciterator-interface
@lorenzofox3 that still works even with the manual implementation. The benefit though is that the code is directly compatible with Node.js 8.
This would be possible by wrapping the
Awaiting
I do like the idea of using different method names! Especially since these iterators aren't immediately useful on Node.js <= 8. I'm struggling with the name though. I can definitely see Sticking with the idea of making the iterator lazy, we could add a |
|
It's not necessarily clear that returns an iterator, though the same applies to |
|
|
I like |
…the same event This clears 'queue' before the iterator pushes to it, which shouldn't cause a crash.
Feature-detect for-await-of support, and only transpile this feature when necessary. With Node.js 9 you can use the following to run tests with the native implementation: ```console $ node --harmony_async_iteration ./node_modules/.bin/ava ```
I've done some more work on this now. I'd like to land #25 so |
This syntax is transformed using async-to-generator, not async-generator-functions.
Conceptually this may conflict with regular listener behavior. #26 makes it explicit that
|
This is no longer a concern with the changes in #27. The TS definition for |
Actually this does bother me. Will explore synchronously delivering data to the iterators, before other listeners are invoked. |
clearListeners() now signals iterators to not expect further items, without clearing the current queue. Clarify that clearListeners() also impacts iterators. Similarly include the number of active iterators in listenerCount(). Enqueue new iterator items synchronously, before listeners are called.
I've resolved the merge conflicts. Iterators are now fed events without using the listener mechanism, given my earlier musings. |
@dinoboff does the |
@novemberborn I doubt it would be required. I will test it. |
@dinoboff Did you have a chance to test it? :) |
I suspect that even if this is an issue with TS 2.7 it'll soon be moot, since async iterators are now stage 4. |
buy the way guys, I have published array like operators on asynchronous iterable ;) |
@novemberborn tries with: mkdir tmp
cd tmp
npm init -y
npm i typescript
npm i lorenzofox3/emittery I used: // index.ts
import Emittery = require('emittery');
const ee = new Emittery();
ee.emit('foo', 'bar'); When compiling with
ps: It works with |
I suspect so. Question then is whether we wait or just document this. |
I think we should document the behavior and land this PR. When TS 2.8 is out we can just update the docs. |
@lorenzofox3 Would you be interested in finishing this? |
sure. give me until Monday |
Actually what do you expect exactly ? If I understand correctly @novemberborn had not merged his continued work due to a typescript issue. So what am I supposed to do ? I think at this point and regarding the size of library, the age of the PR and the fact it has no dependency, it might be easier to (re)write with modern ES (and maybe directly in typescript as you want type declarations) and let the transpilation to the consumer of the library. Who is actually using legacy.js ? If one wants to build a legacy application using emittery it will anyway probably transpile it himself to avoid the duplication of the async to generator babel helper shipped within legacy.js ? I find the set up quite sophisticated and not very dev friendly for a somewhat simple library |
The TypeScript thing should not be an issue anymore.
I think the remaining work is to fix the merge conflicts and document that the methods that return an async iterator require Node.js 12. |
Actually with the current implementation (ie latest commit on this PR), what requires a given version of node (8.10 with flags or above) is the However if you want to swap with a more modern implementation (using async generator) we will need to drop the support for node 8 as the code of emittery itself would throw syntax error. |
You can use
I don't want that yet. We can open an issue about it after this PR is merged so we can move to a async generator in the future. |
closed in favor of #40 (I had deleted the fork repo) |
Note: